fix: allow statement-level consts in named argument shorthand instead of panicking#9814
fix: allow statement-level consts in named argument shorthand instead of panicking#9814
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
|
This change is |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 807d98d7c7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let Expr::Var(expr_var) = &res { | ||
| let item = ResolvedGenericItem::Variable(expr_var.var); | ||
| ctx.resolver.data.resolved_items.generic.insert(identifier.stable_ptr(ctx.db), item); | ||
| } |
There was a problem hiding this comment.
When resolve_variable_by_name resolves a statement-level const (Expr::Constant), this new if let Expr::Var path skips inserting any entry into resolved_items. As a result, identifier lookups that rely on lookup_resolved_generic_item_by_ptr / lookup_resolved_concrete_item_by_ptr cannot resolve :x/{ x } shorthand uses of local consts for tooling (e.g., definition/debug mapping), even though the expression now compiles. resolve_expr_path already handles this by inserting a concrete constant entry, so this shorthand path should mirror that behavior.
Useful? React with 👍 / 👎.
orizi
changed the base branch from
orizi/03-31-fix_modifier_terminal_text
to
graphite-base/9814
… of panicking Fixes #9789
orizi
force-pushed
the
graphite-base/9814
branch
from
a8da0fc to
3855e3f
Compare
orizi
force-pushed
the
orizi/03-31-fix_statement_const_in_named_arg_shorthand
branch
from
807d98d to
f9dc370
Compare
2 participants
Summary
Fixed a panic in variable resolution by replacing
extract_matches!macro with safe pattern matching when resolving variables by name. Added test case for statement-level const used in named argument shorthand.Type of change
Please check one:
Why is this change needed?
The
extract_matches!macro was causing a panic when the resolved expression was not a variable expression (Expr::Var). This could occur in certain edge cases where variable resolution returns a different expression type, leading to runtime crashes during semantic analysis.What was the behavior or documentation before?
The code used
extract_matches!(&res, Expr::Var).varwhich would panic if the resolved expressionreswas not of typeExpr::Var, causing the compiler to crash unexpectedly.What is the behavior or documentation after?
The code now uses safe pattern matching with
if let Expr::Var(expr_var) = &resto only insert the resolved item when the expression is actually a variable, preventing panics and allowing the function to continue gracefully in all cases.Related issue or discussion (if any)
Fixes #9789
Additional context
The test case demonstrates that statement-level constants used in named argument shorthand should work without diagnostics, which helps ensure this fix doesn't break legitimate use cases.
Note
Medium Risk
Touches core name/variable resolution in semantic analysis; change is small but could affect how identifiers are recorded for downstream resolution and diagnostics.
Overview
Fixes a compiler panic during expression semantic computation by making
resolve_variable_by_nameonly record aResolvedGenericItem::Variablewhen the resolved expression is actuallyExpr::Var, instead of unconditionallyextract_matches!-ing.Adds a regression test proving statement-level
constvalues can be used with named argument shorthand (e.g.takes_val(:x)) without diagnostics.Written by Cursor Bugbot for commit f9dc370. This will update automatically on new commits. Configure here.